Repository navigation
Conversation
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
Walkthrough
Priority: Merge Risk:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
products/workflows/backend/services/email_sender_verification.py (1)
11-29: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftBound the sweep cost: it has no time limit, no overlap guard, and no cap on abandoned senders.
The query selects every SES sender with
verified=False. Senders that never complete DNS setup stay in this set permanently. Each unique (team, domain, subdomain) key costs several SES calls and a DMARC lookup with a 5-second lifetime. It also builds a newSESProvider(), which creates five boto3 clients. The sweep is sequential and the task expires after 300 seconds. As the pending set grows, one run can take longer than the 5-minute schedule. Runs then overlap on theLONG_RUNNINGqueue and repeat the same SES calls.Suggested fixes:
- Add a
soft_time_limitand a single-flight lock (for example a Redis lock) to the task.- Skip or back off senders that have been pending for a long time (for example older than N days).
- Reuse one
SESProviderper sweep instead of one per sender.
[medium_effort_placeholder]
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
- Review profile: QUIET
- Plan: Enterprise
- Run ID:
baef3df4-319e-45c2-a144-aac79d1b6bf8
📒 Files selected for processing (11)
posthog/models/integration/email.pyposthog/tasks/scheduled.pyproducts/workflows/backend/facade/email.pyproducts/workflows/backend/facade/tasks.pyproducts/workflows/backend/providers/ses.pyproducts/workflows/backend/services/email_sender_verification.pyproducts/workflows/backend/tasks/__init__.pyproducts/workflows/backend/tasks/email_sender_verification.pyproducts/workflows/backend/test/test_email_integration_ses.pyproducts/workflows/backend/test/test_email_sender_verification.pyproducts/workflows/backend/test/test_ses_provider.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 3 remain after this review.
|
Note 🤖 GPT-6.1 Sol responding on behalf of Michael @coderabbitai Fixed in ea4acf1 and 21e71d6. The sweep now uses a token-owned Redis lease, a four-minute work budget, a 270-second hard task limit, and saved progress over a bounded sender pass. Older pending senders remain eligible for retries even while new senders arrive; the task/API tests cover overlap and retry fairness. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git show --stat --oneline ea4acf1b
git show --stat --oneline 21e71d6d
ast-grep outline products/workflows/backend/services/email_sender_verification.py --items all
ast-grep outline products/workflows/backend/tasks/email_sender_verification.py --items all
ast-grep outline products/workflows/backend/test/test_email_sender_verification.py --items all
sed -n '1,300p' products/workflows/backend/services/email_sender_verification.py
sed -n '1,220p' products/workflows/backend/tasks/email_sender_verification.py
rg -n -C 5 'lease|overlap|fair|retry|progress|budget|time_limit|lock|cursor|bounded' products/workflows/backend/test/test_email_sender_verification.pyLength of output: 7475 🏁 Script executed: #!/bin/bash
set -euo pipefail
sed -n '29,270p' products/workflows/backend/test/test_email_sender_verification.pyLength of output: 9094
You are interacting with an AI system. |
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
products/workflows/backend/services/email_sender_verification.py (1)
43-43: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdvance
CURSOR_KEYafter the refresh, not before it.Line 43 writes
integration.idtoCURSOR_KEYbeforerefresh_verification()runs. The task hastime_limit=270. If Celery hard-kills the task during an SES or DNS call, the cursor already points at the unchecked integration. The next run then filtersid__gt=cursorand skips that integration until the next full pass. Write the cursor after the refresh. The budgetreturnat Line 42 then keeps resuming at the unprocessed row, and a killed refresh is retried.Proposed fix
if monotonic() - started_at >= EmailSenderVerification.WORK_BUDGET_SECONDS: return - redis.set(EmailSenderVerification.CURSOR_KEY, integration.id) domain = integration.config.get("domain") mail_from_subdomain = integration.config.get("mail_from_subdomain", "feedback") if not domain: + redis.set(EmailSenderVerification.CURSOR_KEY, integration.id) continue key = (integration.team_id, domain, mail_from_subdomain) if key in checked: + redis.set(EmailSenderVerification.CURSOR_KEY, integration.id) continue checked.add(key) try: EmailIntegration(integration).refresh_verification() except Exception: logger.exception(...) + redis.set(EmailSenderVerification.CURSOR_KEY, integration.id)
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
- Review profile: QUIET
- Plan: Enterprise
- Run ID:
37d23168-5101-4170-9d8a-11d62d2ddd0f
📒 Files selected for processing (3)
products/workflows/backend/services/email_sender_verification.pyproducts/workflows/backend/tasks/email_sender_verification.pyproducts/workflows/backend/test/test_email_sender_verification.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 3 remain after this review.
|
Risk: No findings The delta since the last review splits a team-scoped sender query into a reusable Sentinel reviewed |
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, please remove the |
Background sweeps check pending SES email senders and save the verified flag, so Channels and MCP reads see a verified sender without the user opening the verification UI. Senders whose verification was requested in the last hour are checked every minute, older ones up to 7 days every 5 minutes. A resolved SES VDM advisor finding marks the pending senders on its domain as requested again. SES status reads are batched per 100 senders and paced in background providers. Matching senders are row-locked while marked verified, and the first verified sender becomes the project's default when none exists. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ed71c30 to
25af2db
Compare
Resolve against the email domain status endpoint and wizard on master. _mark_domain_verified stays the one writer of verified: it locks the senders that share the checked MAIL FROM subdomain, sets the flag with a JSON update and reports the first verification. The background sweep reaches it through apply_verification, without a verified report. The SES provider keeps master's status read clients and the background client config, paced reads and batched identity statuses. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…om domain in status Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Moved to #127146, which runs on a PostHog branch so CI can run. |
Note
Merged
masterine4095eb6a8ato resolve conflicts with the email domain wizard stack (#120402 and later). The conflict resolutions are the commit range to review:25af2dbab43..e4095eb6a8a.status()keeps master's read-only shape. Onlyverify(), the background sweep and the advisor path save verification.verify()sendsemail domain verifiedthrough master's_report_email_domain_verifiedon the first verification. The background sweep sends no event.287950a29f1:email domain verifiedfires once per domain across MAIL FROM subdomains, as on master. The status check matches a MAIL FROM domain that differs in case or by a trailing dot.Problem
Workflows users can finish DNS setup and still see their sender as unverified until they open its verification UI. After this PR, a new sender shows as verified within about a minute of SES accepting its DNS.
Impact
Pays into Grow Workflows through Workflows activation rate. A sender that verifies without the user on the page removes a return visit before the first send, and the upcoming "send once verified" change builds on this sweep so a waiting workflow can go live by itself. Expected move on its own: small, up.
Warning
🟡 Merge risk: medium · one-way door for saved verification flags and sender defaults
verification_requested_attimestamp, visible in the integrations API. Creating a sender, a pending verify click and a resolved advisor finding set it.Changes
created_at. Each tier has its own lock and cursor.Before:
flowchart LR UI[Open verification UI] --> Check[Check SES and DNS] Check --> Saved[Save verified flag] Saved --> Reads[UI and MCP reads] classDef phYellow fill:#f9bd2b,stroke:#f9bd2b,color:#000; classDef phRed fill:#f54e00,stroke:#f54e00,color:#fff; classDef phGray fill:#e5e7eb,stroke:#c7ccd1,color:#000; class UI,Reads phYellow; class Check phRed; class Saved phGray;After:
flowchart LR Beat[Every minute, under 1 hour old] --> Guard[Acquire tier lease] Beat5[Every 5 minutes, 1 hour to 7 days old] --> Guard Guard --> Pending[Resume pass over the tier] Advisor[SES advisor finding resolved] --> Queue[Mark pending senders requested] Queue --> Guard Pending --> Batch[Read SES status for 100 senders] Batch --> Check[Check DNS and tenant for ready senders] Check --> Saved[Update matching verified flags] Saved --> Reads[UI and MCP reads] Saved --> Workers[Reload sending workers] classDef phYellow fill:#f9bd2b,stroke:#f9bd2b,color:#000; classDef phBlue fill:#1d4aff,stroke:#1d4aff,color:#fff; classDef phRed fill:#f54e00,stroke:#f54e00,color:#fff; classDef phGray fill:#e5e7eb,stroke:#c7ccd1,color:#000; class Beat,Beat5,Advisor,Reads phYellow; class Guard,Pending,Queue,Batch,Workers phBlue; class Check phRed; class Saved phGray;Warning
The advisor trigger needs AWS-side setup before it fires. The SES EventBridge rule that feeds the SES events webhook must also match
Advisor Recommendation Status Resolved, and VDM must be on for the account. Until then, the two sweeps carry the load.Note
Known gaps, not fixed here:
How did you test this code?
Test seams: the background Celery task, integration reads used by API/MCP, and the project API's default sender. Tests fake AWS, DNS, and elapsed time.
Test rationale: The closest existing test,
test_email_verify_updates_integration, exercises explicit verification and mocks the facade. It cannot catch a missing background refresh. The task/API cases cover verification, incomplete checks, concurrent edits, failure recovery, overlapping sweeps, and retry fairness. Extended cases preserve default selection after merging master and reject an edited sender when its sibling verifies.The review round added cases for batched status reads, stale senders, MAIL FROM case, a budget run out by slow SES calls, and the verify button skipping senders on another MAIL FROM subdomain. Each case fails when its fix is reverted.
The tier round added age-boundary cases for both sweeps, a per-domain check that ignores age and domain case, and a webhook case where an identity named
team-5.example.comreaches the sender check, not the tenant sync. Each fails when its rule is reverted.Master refresh on
25af2dba: the touched-area suites (sender sweep, SES provider, SES integration, SES events webhook,TestEmailIntegration, scheduler, task registration, team workflows config) passed locally, 112 tests.test_ses_provider.pyruns on its own, away from the known logging-setup clash below. One drift fix: the sweep tests no longer clear the per-processTeam.workflows_configcache, because #118777 removed it.hogli product:lint workflowsandhogli ci:preflight --strictpassed. Repository-wide mypy was not rerun after the refresh.Local red/green proof and verification commands (before the master refresh)
Before the implementation, SES and DNS reported success but the integration API returned false:
The read-only and concurrent-edit cases also failed before their fixes. The task now updates the API status without calling SES setup methods.
The stronger overlap test fails when a temporary test plugin bypasses the lock. The budget test fails without a fixed pass boundary:
No frontend code or layout changes. Not checked: real SES, public DNS propagation, production mail delivery, or a real VDM advisor event. The advisor event shape follows the SES EventBridge docs. Three
TestGetIdentityIspMetricscases fail when they share a process withposthog/tasks/test/test_task_registration.py, which re-runs Django's logging setup and disables existing loggers. This happens on master too and gets its own fix PR. These tests do not read event properties.Release status
Docs update
None. No existing document under
docs/covers sender verification.🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Involvement: no grilling rounds or prototypes in this refresh; earlier rounds ran two QA swarms and three single reviews; proof is the local test output above.
Agent: T3 Code / Codex,
gpt-6.1-sol. Review-round fixes: T3 Code / Claude Code,claude-opus-5-5.Skills:
tdd,writing-tests,hogli,running-ci-preflight,qa-swarm,pr,writing-pr-descriptions,merge-risk,writing-voice,unslop,principle-prove-it-works.Two QA rounds fixed overlapping sweeps, ordered failure mocks, an insensitive overlap test, and retry starvation during continuous arrivals. All findings are fixed or rejected.
The deferred-import comment request was rejected under the user's explicit no-code-comments instruction. GPT-6.1 Sol reviewed correctness and risk; GPT-6 Astra covered fresh eyes and design after Opus returned 429. CodeRabbit CLI skipped because it is signed out.
The master refresh used a merge commit. A read-only Claude Opus 5.5 review found that a changed sender could become the default when a sibling verified. The fix requires the checked sender to match the verification update; a failing task/API case proves the guard. The same guard excludes a sender deleted during the provider check.
A QA swarm (GPT-6.1 Sol: correctness, risk, design; Claude Opus 5.5: fresh eyes) on the tier round found a default-sender race, a retry count of three, unpaced tenant reads, parallel advisor readers, and dropped unfinished domain checks. All are fixed. The narrow fix round then found that the Redis advisor queue added in round 3 could starve, drop events, and drop failed checks. The queue was replaced by a per-sender
verification_requested_attimestamp that the tier sweeps already process. That also covers re-added and re-verified senders. Hardening the queue with event generations was the alternative.A critical Claude Opus 5.5 review after the merge flagged unbatched SES reads, an unbounded sweep, unbounded SES calls, and case-sensitive MAIL FROM matching. All four are fixed. The read-only check moved into the workflows service, which removed the duplicate SES path on the integration model and the one-function facade module. Three lower-risk findings are listed as known gaps.
soft_time_limitwas rejected: Celery raises it as anException, which the per-sender guard would swallow. Client timeouts and the per-step budget bound the run instead.SES sends no EventBridge event when a domain identity, DKIM or MAIL FROM becomes verified. A resolved VDM advisor finding is the closest signal, so it only triggers a fresh check. The sweeps stay as the source of truth.
GitHub CI requires maintainer approval; CI and patch coverage remain unchecked. GitHub denied the requested assignee, so the PR remains unassigned.
A Claude Opus 5.5 (
claude-opus-5-5) refresh moved the PR onto current master as one commit. Master had added a default reply-to on senders (#115898), turned off SES feedback forwarding (#113335), and registered new workflows task modules. All three merged without semantic conflicts. The per-team-instance config load from #118777 needed one test setup change. Skills:modernize-pr,pr-description.The duplicate search found #110516 (on-demand domain status reads) and #113059 (verification access scope). Both closed without merging, and no merged PR adds a background sweep. Fixtures contain invented domains, addresses and tokens.